Provide a way to terminate unfinished requests after graceful shutdown #5941

ikhoon · 2024-10-16T06:33:21Z

Motivation:

Unfinished requests even after graceful shutdown period are forcibly closed with ClosedSessionException. As ClosedSessionException indicates that the connection was unexpectedly disconnected, ClosedSessionException is not suitable for graceful shutdown.

In this PR, I propose to add ShuttingDownException to terminate unfinished requests when a server stops.

Modifications:

Introduce GracefulShutdown to customize graceful shutdown behavior.
- Users can specify a error function to create an exception to unfinished terminate requests.
Fixed HttpServerHandler to send error responses using the error function of GracefulShutdown
Fixed Server to send error respones first and then close the connnections.
Deprecation) ServerConfig.gracefulShutdownQuietPeriod() and ServerConfig.gracefulShutdownTimeout() have been deprecated in favor of ServerConfig.gracefulShutdown().

Result:

You can now use GracefulShutdown to terminate unfinished requests when a server stops.

GracefulShutdown gracefulShutdown =
  GracefulShutdown
    .builder()
    .quietPeriod(Duration.ofSeconds(10))
    .timeout(Duration.ofSeconds(15))
    .shutdownErrorFunction((ctx, req) -> {
        return new ServerStopException();
    })
    .build();

Server
  .builder()
  .gracefulShutdown(gracefulShutdown);

Motivation: Unfinished requests even after graceful shutdown period are forcivily closed with `ClosedSessionException`. As `ClosedSessionException` indicates that the connection was unexpectedly disconnected, `ClosedSessionException` is not suitable for graceful shutdown. In this PR, I propose to add `ShuttingDownException` to terminate unfinished requests when a server stops. Modifications: - Introduce `GracefulShutdown` to customize graceful shutdown behavior. - Users can specify a error function to create an exception to unfinished terminate requests. - Fixed `HttpServerHandler` to send error responses using the error function of `GracefulShutdown` - Fixed `Server` to send error respones first and then close the connnections. - Deprecation) `ServerConfig.gracefulShutdownQuietPeriod()` and `ServerConfig.gracefulShutdownTimeout()` have been deprecated in favor of `ServerConfig.gracefulShutdown()`. Result: You can now use `GracefulShutdown` to terminate unfinished requests when a server stops. ```java GracefulShutdown gracefulShutdown = GracefulShutdown .builder() .quietPeriod(Duration.ofSeconds(10)) .timeout(Duration.ofSeconds(15)) .shutdownErrorFunction((ctx, req) -> { return new ServerStopException(); }) .build(); Server .builder() .gracefulShutdown(gracefulShutdown); ```

github-actions · 2024-10-16T06:47:24Z

🔍 Build Scan® (commit: `f3f9b1f`)

Job name	Status	Build Scan®
build-ubicloud-standard-8-jdk-8	✅	https://ge.armeria.dev/s/ytcuyvikygk56
build-ubicloud-standard-8-jdk-21-snapshot-blockhound	✅	https://ge.armeria.dev/s/aps7ijzua3p2g
build-ubicloud-standard-8-jdk-17-min-java-17-coverage	✅	https://ge.armeria.dev/s/yeml4gvwm77ru
build-ubicloud-standard-8-jdk-17-min-java-11	✅	https://ge.armeria.dev/s/voyhpybbh5sj4
build-ubicloud-standard-8-jdk-17-leak	✅	https://ge.armeria.dev/s/dynijahxidow2
build-ubicloud-standard-8-jdk-11	❌ (failure)	https://ge.armeria.dev/s/ldqtccbuke3sk
build-macos-latest-jdk-21	✅	https://ge.armeria.dev/s/ieamd53bc3tz6

trustin · 2024-10-23T08:37:16Z

core/src/main/java/com/linecorp/armeria/server/GracefulShutdown.java

+    /**
+     * Returns the quiet period to wait for active requests to go end before shutting down.
+     * {@link Duration#ZERO} means the server will stop right away without waiting.
+     */
+    Duration quietPeriod();
+
+    /**
+     * Returns the amount of time to wait before shutting down the server regardless of active requests.
+     * This should be set to a time greater than {@code quietPeriod} to ensure the server shuts down even
+     * if there is a stuck request.
+     */
+    Duration timeout();


Should we make this even more flexible rather than just letting a user configure the quiet period and timeout? For example:

interface GracefulShutdown { ... // Armeria core passes its GracefulShutdownHandler to this method. void startGracefulShutdown(GracefulShutdownHandler handler); } // A GracefulShutdown implementation calls back Armeria core via this handler. interface GracefulShutdownHandler { void gracefulShutdownStarted(...); void quietPeriodComplete(...); void gracefulShutdownComplete(...); @Nullable Throwable toException(ctx, req); }

I've been thinking about the design of GracefulShutdownHandler, but I still don't have a good idea yet of what parameters should be set in its methods.

Should we introduce GracefulShutdownHandler when there is a requirement to customize the low-level graceful shutdown logic?

Sure! No problem.

jrhee17

Looks good overall, left a minor question on thread safety when closing connections

jrhee17 · 2024-11-05T06:13:38Z

core/src/main/java/com/linecorp/armeria/server/Server.java

+            for (Channel ch : children) {
+                final HttpServerHandler serverHandler = ch.pipeline().get(HttpServerHandler.class);
+                if (serverHandler != null) {
+                    closeFutures.add(serverHandler.shutdown(ch));


This is called from the startStopExecutor, which breaks the assumption that HttpServerHandler#cleanup is called from the event loop assigned to the channel.

Is this intentional?

Nice catch, it was my mistake.

jrhee17 · 2024-11-05T06:23:09Z

core/src/main/java/com/linecorp/armeria/server/Server.java

-                        if (bossGroups.isEmpty()) {
-                            finishDoStop(future);
-                            return;
+                shutdownServerHandlers().handle((unused3, unused4) -> {


I understood now, that requests are closed with an exception once before closing the socket directly.

As a result, users will see more 503 responses instead of connection resets

jrhee17 · 2024-11-05T06:36:39Z

core/src/main/java/com/linecorp/armeria/server/HttpServerHandler.java

+                                          .map(DecodedHttpRequest::whenResponseSent)
+                                          .toArray(CompletableFuture[]::new);
+                CompletableFuture.allOf(futures).handle((unused0, unused1) -> {
+                    completionFuture.complete(null);


Note: checked that the order of completing the future vs. close the encoder doesn't really matter as 1) pipeline requests are aborted above 2) the timing of closing the keepAliveHandler doesn't really matter

ikhoon · 2024-11-07T08:56:47Z

Changed the target milestone to 1.32.0. I need some time to figure out how to implement the API that Trustin suggested.

trustin

Nice work!

minwoox

Looks great. 👍
Left some minor comments.

minwoox · 2024-12-09T02:03:22Z

core/src/main/java/com/linecorp/armeria/server/GracefulShutdownBuilder.java

+     * Builds a new {@link GracefulShutdown} with the configured parameters.
+     */
+    public GracefulShutdown build() {
+        validateGreaterThanOrEqual(timeout, "timeout", quietPeriod, "quietPeriod");


How about moving validateGreaterThanOrEqual into this class?

minwoox · 2024-12-09T02:19:18Z

core/src/main/java/com/linecorp/armeria/server/Server.java

+            // The future returned by shutdown() will be always completed successfully.
+            final CompletableFuture<List<Void>> combined = CompletableFutures.allAsList(closeFutures);
+            config.workerGroup().schedule(() -> {
+                combined.complete(ImmutableList.of());


This won't wait all closeFutures to be completed if completing closeFutures takes more than 1 second.

It was intentional. A graceful timeout has passed since the stop process started. 1 second is the last chance/time to send shutting-down responses before the connections are closed.

If it was intentional, let's not call CompletableFutures.allAsList(closeFutures); because it's useless. We can do:

final CompletableFuture<Void> future = new CompletableFuture<>(); config.workerGroup().schedule(() -> { future.complete(); }, 1, TimeUnit.SECONDS); return future;

Oops, NVM. It is still needed because the future can be completed earlier.

…nto shutting-down-exception

minwoox

Thanks!

ikhoon added the new feature label Oct 16, 2024

ikhoon added this to the 1.31.0 milestone Oct 16, 2024

ikhoon added the deprecation label Oct 16, 2024

lint

7277f60

ikhoon and others added 4 commits October 18, 2024 10:24

Merge branch 'main' into shutting-down-exception

9d83337

Move ShuttingDownException to common and fix a broken test

685f93d

lint

227b6b4

Merge branch 'main' into shutting-down-exception

0a40aa8

ikhoon marked this pull request as ready for review October 23, 2024 02:52

ikhoon requested review from jrhee17, minwoox and trustin as code owners October 23, 2024 02:52

trustin reviewed Oct 23, 2024

View reviewed changes

jrhee17 approved these changes Nov 5, 2024

View reviewed changes

ikhoon modified the milestones: 1.31.0, 1.32.0 Nov 7, 2024

execute in event loops

538deba

ikhoon marked this pull request as draft November 22, 2024 05:42

ikhoon and others added 2 commits December 5, 2024 18:52

rename shutdownError to toException

bc8de5a

Merge branch 'main' into shutting-down-exception

b45619c

ikhoon marked this pull request as ready for review December 5, 2024 10:04

Merge branch 'main' into shutting-down-exception

d2ba113

trustin approved these changes Dec 6, 2024

View reviewed changes

minwoox reviewed Dec 9, 2024

View reviewed changes

ikhoon added 2 commits December 11, 2024 12:00

move validateGreaterThanOrEqual

69de8be

Merge branch 'shutting-down-exception' of github.com:ikhoon/armeria i…

f3f9b1f

…nto shutting-down-exception

minwoox approved these changes Dec 11, 2024

View reviewed changes

ikhoon merged commit 468aec1 into line:main Dec 11, 2024
13 of 14 checks passed

ikhoon deleted the shutting-down-exception branch December 11, 2024 08:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provide a way to terminate unfinished requests after graceful shutdown #5941

Provide a way to terminate unfinished requests after graceful shutdown #5941

ikhoon commented Oct 16, 2024 •

edited by trustin

Loading

github-actions bot commented Oct 16, 2024 •

edited

Loading

trustin Oct 23, 2024

ikhoon Dec 5, 2024

trustin Dec 6, 2024

jrhee17 left a comment

jrhee17 Nov 5, 2024

ikhoon Nov 7, 2024

jrhee17 Nov 5, 2024

jrhee17 Nov 5, 2024

ikhoon commented Nov 7, 2024

trustin left a comment

minwoox left a comment

minwoox Dec 9, 2024

minwoox Dec 9, 2024

ikhoon Dec 11, 2024 •

edited

Loading

minwoox Dec 11, 2024

minwoox Dec 11, 2024

minwoox left a comment

Provide a way to terminate unfinished requests after graceful shutdown #5941

Provide a way to terminate unfinished requests after graceful shutdown #5941

Conversation

ikhoon commented Oct 16, 2024 • edited by trustin Loading

github-actions bot commented Oct 16, 2024 • edited Loading

🔍 Build Scan® (commit: f3f9b1f)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrhee17 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ikhoon commented Nov 7, 2024

trustin left a comment

Choose a reason for hiding this comment

minwoox left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ikhoon Dec 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

minwoox left a comment

Choose a reason for hiding this comment

ikhoon commented Oct 16, 2024 •

edited by trustin

Loading

github-actions bot commented Oct 16, 2024 •

edited

Loading

🔍 Build Scan® (commit: `f3f9b1f`)

ikhoon Dec 11, 2024 •

edited

Loading